[SPARK-57929][CORE][SQL] Support Binary/List view types in ArrowColumnVector#57210
[SPARK-57929][CORE][SQL] Support Binary/List view types in ArrowColumnVector#57210robert3005 wants to merge 2 commits into
Conversation
|
Can't find anything in the logs about the failure, looks like github flake |
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
Thank you @robert3005, cc @cloud-fan PTAL
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Review: 0 blocking, 1 non-blocking, 0 nits. Clean, well-scoped extension. I verified the manual view-buffer decode in StringViewAccessor byte-for-byte against Arrow's own BaseVariableWidthViewVector.getData (arrow-vector 19.0.0) — the length/bufferIndex/offset positions and the inline vs long branches all match; it's correct.
Suggestions (non-blocking)
ArrowColumnVector.java:560— see inline comment on the untested multi-buffer path.
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Re-review: 1 addressed, 0 remaining, 0 new.
The multi-buffer test coverage flagged in the prior review is now addressed — string_view with multiple data buffers forces the variadic data buffers to spill (setInitialCapacity(16, 8) + assert(getDataBuffers.size() > 1)) and reads every value, genuinely exercising the non-zero buffer-index branch of the accessor. No new findings.
Verification
Re-verified StringViewAccessor's manual view-struct decode byte-for-byte against Arrow's own BaseVariableWidthViewVector.getData (arrow-vector 19.0.0): length at rowId*16, inline value at +LENGTH_WIDTH (4), and for long values bufferIndex at +8 and offset at +12 before indexing getDataBuffers().get(bufferIndex) — offsets, constants, and the <= INLINE_SIZE (12) inline boundary all match. The BinaryAccessor/ArrayAccessor generalizations are type-sound (all three binary vectors implement VariableWidthFieldVector; ListViewVector satisfies BaseListVector & RepeatedValueVector).
LGTM.
…nVector ### What changes were proposed in this pull request? Add support for constructing ArrowColumnVector from String/BinaryView and ListView ### Why are the changes needed? Arrow in spec 1.4 has added these as more efficient way to represent list and string types in memory ### Does this PR introduce _any_ user-facing change? No, this is internal developer api ### How was this patch tested? Added Tests ### Was this patch authored or co-authored using generative AI tooling? Yes, this pr was authored with use of Fable 5 Closes #57210 from robert3005/arrow-view-column-vectors. Authored-by: Robert Kruszewski <github@robertk.io> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 544704c) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
3 participants
What changes were proposed in this pull request?
Add support for constructing ArrowColumnVector from String/BinaryView and ListView
Why are the changes needed?
Arrow in spec 1.4 has added these as more efficient way to represent list and string types in memory
Does this PR introduce any user-facing change?
No, this is internal developer api
How was this patch tested?
Added Tests
Was this patch authored or co-authored using generative AI tooling?
Yes, this pr was authored with use of Fable 5